Enable custom training job in SageMaker plugin #113

bnsblue · 2020-08-13T22:39:06Z

TL;DR

This PR adds a flyteplugin to properly generate the CRD for SageMaker custom training job and monitor the status of the submitted job.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

See flyteorg/flyte#451

Tracking Issue

https://github.com/lyft/flyte/issues/

Follow-up issue

NA
OR
https://github.com/lyft/flyte/issues/

…anism; some other smaller refactoring

codecov-commenter · 2020-08-14T06:53:21Z

Codecov Report

Merging #113 into master will increase coverage by 3.12%.
The diff coverage is 71.20%.

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   58.75%   61.87%   +3.12%     
==========================================
  Files          99      104       +5     
  Lines        5957     6434     +477     
==========================================
+ Hits         3500     3981     +481     
+ Misses       2106     2084      -22     
- Partials      351      369      +18

Impacted Files	Coverage Δ
go/tasks/plugins/k8s/sagemaker/config/config.go	`0.00% <0.00%> (ø)`
go/tasks/plugins/k8s/sagemaker/outputs.go	`20.51% <20.51%> (ø)`
...sks/plugins/k8s/sagemaker/hyperparameter_tuning.go	`61.23% <61.23%> (ø)`
go/tasks/plugins/k8s/sagemaker/custom_training.go	`68.42% <68.42%> (ø)`
go/tasks/plugins/k8s/sagemaker/builtin_training.go	`72.18% <72.18%> (ø)`
go/tasks/plugins/k8s/sagemaker/plugin.go	`73.07% <73.07%> (ø)`
go/tasks/plugins/k8s/sagemaker/utils.go	`72.76% <80.43%> (+10.18%)`	⬆️
...o/tasks/plugins/k8s/sagemaker/plugin_test_utils.go	`95.93% <96.72%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af40d96...d64afce. Read the comment docs.

… plugin

… can access the DataStore and such

go/tasks/plugins/awsutils/awsutils.go

go/tasks/pluginmachinery/k8s/plugin.go

go/tasks/plugins/k8s/sagemaker/outputs.go

go/tasks/plugins/k8s/sagemaker/sagemaker.go

go/tasks/plugins/k8s/sagemaker/utils.go

…and built-in training plugin

kumare3 · 2020-09-02T16:34:41Z

@bnsblue can we update the description?

go/tasks/plugins/k8s/sagemaker/constants.go

go/tasks/plugins/k8s/sagemaker/utils.go

go/tasks/plugins/k8s/sagemaker/sagemaker.go

…ptimize for readability

kumare3 · 2020-09-02T23:04:48Z

go/tasks/plugins/k8s/sagemaker/builtin_training.go

+	return trainingJob, nil
+}
+
+func (m awsSagemakerPlugin) getTaskPhaseForTrainingJob(


is this different for every job type? or are the jobSTatus common?

The output handling is different for different type of tasks

the job statuses are not common either

Ya I realized once I went through all of them

go/tasks/plugins/k8s/sagemaker/custom_training.go

kumare3 · 2020-09-02T23:12:47Z

go/tasks/plugins/k8s/sagemaker/custom_training.go

+			OutputDataConfig: &commonv1.OutputDataConfig{
+				S3OutputPath: ToStringPtr(outputPath),
+			},
+			CheckpointConfig: nil,


so this is one thing we might want to model in the future. Passing in a custom checkpoint config?

yes we will do this soon

kumare3 · 2020-09-02T23:15:22Z

go/tasks/plugins/k8s/sagemaker/plugin.go

+func (m awsSagemakerPlugin) getEventInfoForJob(ctx context.Context, job k8s.Resource) (*pluginsCore.TaskInfo, error) {
+
+	var jobRegion, jobName, jobTypeInURL, sagemakerLinkName string
+	if m.TaskType == trainingJobTaskType {


should we move these parts as submethods in the specific files? co-located code?

I don't quite get what you mean. Mind to elaborate?

So when task type == custom that code block could be in the custom.go file

Done refactoring

kumare3 · 2020-09-02T23:18:39Z

@bnsblue This is awesome work! Thank you. Couple comments from my side

bnsblue · 2020-09-03T02:25:52Z

Would you mind elaborating some of your comments?
Also could you please take another look? @kumare3

kumare3 · 2020-09-03T02:26:37Z

Will do

bnsblue · 2020-09-03T03:17:56Z

@kumare3 please take another look

kumare3

Looks great to me!

* checking key existence in GetRole() * (training job plugin only) inject hp; refactor image uri getting mechanism; some other smaller refactoring * change the key and value of the flyte sm command * get rid of a obsolete function * add and change unit tests * add unit tests * lint * refactor a dummy object generation function * refactor a dummy object generation function * add unit tests * lint * forming the runner cmd and inject it as a hyperparameter in SageMaker plugin * fix unit tests * fix unit tests * add custom training plugin * revert overriden changes * compose the __FLYTE_SAGEMAKER_CMD__ in custom training job plugin * add unit test * lint * fix getEventInfoForJob * fix image getting * fix unit tests * stick with inputs.pb and modify hp injecting logic accordingly * fix args converting logic * use default file-based output for custom training job * expanding PluginContext interface with necessary methods so SM plugin can access the DataStore and such * lint error * add unit tests * add logic to inject env vars into hyperparameters * fix output prefix * fix output prefix * remove job name from output prefix for now * fix a unit test * accommodating new arg and env var passsing syntax * injecting a env var to force disable statsd for sagemaker custom training * correcting variable name * remove unused constant * remove comments * fix unit tests * merge template.go * pr comments * add guarding statement wrt algorithm name for custom training plugin and built-in training plugin * refactor file structures: splitting the code into multiple files to optimize for readability * add documentations to a set of constants, and fix a constant's name * split tests into multiple files * correcting error types: make permanent failures * refactor

bnsblue added 2 commits August 12, 2020 11:41

checking key existence in GetRole()

7ffa794

(training job plugin only) inject hp; refactor image uri getting mech…

3a8be98

…anism; some other smaller refactoring

bnsblue mentioned this pull request Aug 13, 2020

Implement or refactor the SageMaker flyteplugin to enable Custom Training Job flyteorg/flyte#451

Closed

6 tasks

bnsblue added 6 commits August 13, 2020 15:44

change the key and value of the flyte sm command

685fbf0

get rid of a obsolete function

65eb0a2

add and change unit tests

43ea822

add unit tests

536f2cf

lint

2a1e007

refactor a dummy object generation function

4de7787

bnsblue added 3 commits August 13, 2020 23:56

refactor a dummy object generation function

5b5e4c4

add unit tests

3a8ee70

lint

688e1f9

bnsblue mentioned this pull request Aug 15, 2020

Add more tests to SageMaker plugin module flyteorg/flyte#461

Closed

bnsblue added 16 commits August 20, 2020 15:32

forming the runner cmd and inject it as a hyperparameter in SageMaker…

b80f943

… plugin

fix unit tests

67275fd

fix unit tests

cd3edaa

add custom training plugin

f1adee2

revert overriden changes

53e517c

compose the __FLYTE_SAGEMAKER_CMD__ in custom training job plugin

c519e93

add unit test

a125e9b

lint

27b35bc

fix getEventInfoForJob

0f3e050

fix image getting

e1c2762

fix unit tests

0062934

stick with inputs.pb and modify hp injecting logic accordingly

837ae99

fix args converting logic

ce23f18

use default file-based output for custom training job

d0a085d

expanding PluginContext interface with necessary methods so SM plugin…

580b18a

… can access the DataStore and such

lint error

46cfcf8

EngHabu reviewed Sep 1, 2020

View reviewed changes

bnsblue added 2 commits September 1, 2020 16:53

pr comments

2a3046c

add guarding statement wrt algorithm name for custom training plugin …

01a5e12

…and built-in training plugin

bnsblue requested review from EngHabu and kumare3 September 2, 2020 02:37

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/constants.go Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/utils.go Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/sagemaker.go Outdated Show resolved Hide resolved

bnsblue added 3 commits September 2, 2020 12:08

refactor file structures: splitting the code into multiple files to o…

6a63de2

…ptimize for readability

add documentations to a set of constants, and fix a constant's name

6a6b0a5

split tests into multiple files

6135f80

bnsblue requested a review from kumare3 September 2, 2020 21:20

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/custom_training.go Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/custom_training.go Outdated Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/custom_training.go Outdated Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

go/tasks/plugins/k8s/sagemaker/custom_training.go Outdated Show resolved Hide resolved

kumare3 reviewed Sep 2, 2020

View reviewed changes

correcting error types: make permanent failures

f46aed9

bnsblue requested a review from kumare3 September 3, 2020 02:25

refactor

d64afce

kumare3 approved these changes Sep 3, 2020

View reviewed changes

bnsblue merged commit adfa4b2 into master Sep 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable custom training job in SageMaker plugin #113

Enable custom training job in SageMaker plugin #113

bnsblue commented Aug 13, 2020 •

edited

Loading

codecov-commenter commented Aug 14, 2020 •

edited

Loading

kumare3 commented Sep 2, 2020

kumare3 Sep 2, 2020

bnsblue Sep 2, 2020

bnsblue Sep 3, 2020

kumare3 Sep 3, 2020

kumare3 Sep 2, 2020

bnsblue Sep 3, 2020

kumare3 Sep 2, 2020

bnsblue Sep 3, 2020

kumare3 Sep 3, 2020

bnsblue Sep 3, 2020

bnsblue Sep 3, 2020

kumare3 commented Sep 2, 2020

bnsblue commented Sep 3, 2020

kumare3 commented Sep 3, 2020

bnsblue commented Sep 3, 2020

kumare3 left a comment

Enable custom training job in SageMaker plugin #113

Enable custom training job in SageMaker plugin #113

Conversation

bnsblue commented Aug 13, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov-commenter commented Aug 14, 2020 • edited Loading

Codecov Report

kumare3 commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumare3 commented Sep 2, 2020

bnsblue commented Sep 3, 2020

kumare3 commented Sep 3, 2020

bnsblue commented Sep 3, 2020

kumare3 left a comment

Choose a reason for hiding this comment

bnsblue commented Aug 13, 2020 •

edited

Loading

codecov-commenter commented Aug 14, 2020 •

edited

Loading